Adjust coingecko prices based on token decimals #2899
Conversation
|
|
||
| let (prices_in_eth, infos) = tokio::try_join!( | ||
| self.bulk_fetch_denominated_in_eth(&tokens), | ||
| self.infos.get_token_infos(&tokens_vec).map(Result::Ok), |
There was a problem hiding this comment.
Would it be possible to change the interface of get_token_infos() to accept a reference to something that implement Iterator? that way we shouldn't need to re-allocate a new vector just to pass it as reference any way 🤔
There was a problem hiding this comment.
Technically a good idea but that doesn't play nice with automock and async_trait unfortunately. :/
| tokens | ||
| .iter() | ||
| .map(|t| { | ||
| let decimals = if *t == usdc { Some(6) } else { Some(18) }; |
There was a problem hiding this comment.
is it possible that a token has more than 18 decimals? if yes, could you please add a token like that in that test to check that the reverse operation (powi with a negative number + then multiplication to normalize it) works as expected? 🙏
There was a problem hiding this comment.
Yeah, good idea. This was actually a problem in the original fix for the oneinch estimator adjustment since we used u64 operations there and 10 ** 21 overflowed that value which caused other issues.
| ) -> Result<HashMap<H160, NativePriceEstimateResult>, PriceEstimationError> { | ||
| tokens.insert(denominator); | ||
| let prices_in_eth = self.bulk_fetch_denominated_in_eth(&tokens).await?; | ||
| tokens.insert(self.denominator.address); |
There was a problem hiding this comment.
Are we deliberately removing the optimisation of not requesting the price for weth if not needed?
There was a problem hiding this comment.
Yes, this was a conscious decision for the sake of simplicity. I'm happy to put the optimization back in if we actually need the extra 5% of batch size but for now I would prefer to keep the code leaner since this file is already on the more complex side IMO.
| @@ -158,19 +178,36 @@ impl CoinGecko { | |||
| token: &Token, | |||
| denominator_price_eth: Decimal, | |||
| prices: &HashMap<Token, f64>, | |||
| infos: &HashMap<Token, TokenInfo>, | |||
There was a problem hiding this comment.
This method signature is quite 🤡 in my opinion (it has 5 values of which some are redundant).
I think we should simply pass it the token as (TokenInfo, priceInEth) and the denominator as (Denominator, priceInEth) and keep this method for the pure non-trivial math.
There was a problem hiding this comment.
The alternative to this function seems to be even more 🤡 error handling in the calling function. While your suggestion would make the conversion more pure it would make the surrounding code way harder to read.
Unless I'm missing some trick to reasonably manage errors (using anonymous closures to get early returns doesn't count IMO).
There was a problem hiding this comment.
I mean the fact that you are passing in denominator_price_eth on top of prices is just redundant (and seems inconsistent) so at least you could either
- remove denominator_price_eth and and "expect" on it being in prices
- pass the token price into the function directly
But I also don't really see how error handling is an issue here (you are bubbling up errors in any case)
| .checked_div(denominator_price_eth) | ||
| .context("division by zero")? | ||
| .checked_mul(adjustment) |
There was a problem hiding this comment.
Do we need to do the adjustment before or after the division to not lose precision?
There was a problem hiding this comment.
I don't think so. Since this is not using a float under the hood but a (base: i64, scale: u32) the order of operations should not matter AFAIU.
| @@ -158,19 +178,36 @@ impl CoinGecko { | |||
| token: &Token, | |||
| denominator_price_eth: Decimal, | |||
| prices: &HashMap<Token, f64>, | |||
| infos: &HashMap<Token, TokenInfo>, | |||
There was a problem hiding this comment.
I mean the fact that you are passing in denominator_price_eth on top of prices is just redundant (and seems inconsistent) so at least you could either
- remove denominator_price_eth and and "expect" on it being in prices
- pass the token price into the function directly
But I also don't really see how error handling is an issue here (you are bubbling up errors in any case)
Description
Fixes: #2887
Changes
Adjust returned native token prices when the quoted token has a different amount of decimals than the native token to be inline with all other native price estimator implementations.
Additionally I also removed the option to quote without denominating in any token mainly to reduce complexity.
How to test
Added an
ignoredunit test that checks that prices make sense.Also compared prices returned for
wxdaiandusdcwith prices logged in our infra.